feat: Add CLI for Unity MCP server #544
Conversation
- Add click-based CLI with 15+ command groups - Commands: gameobject, component, scene, asset, script, editor, prefab, material, lighting, ui, audio, animation, code - HTTP transport to communicate with Unity via MCP server - Output formats: text, json, table - Configuration via environment variables or CLI options - Comprehensive usage guide and unit tests
Reviewer's GuideIntroduce a new Sequence diagram for unity-mcp gameobject create commandsequenceDiagram
actor User
participant CLI as unity_mcp_CLI
participant GameobjectCmd as gameobject_create_command
participant Config as CLIConfig
participant Conn as run_command
participant HTTP as /api/command
participant Hub as PluginHub
participant Unity as Unity_Editor_instance
User->>CLI: unity-mcp gameobject create "MyCube" --primitive Cube
CLI->>Config: set_config from CLI options
CLI->>GameobjectCmd: dispatch click subcommand
GameobjectCmd->>Config: get_config
GameobjectCmd->>Conn: run_command("manage_gameobject", params, config)
Conn->>HTTP: POST /api/command {type: manage_gameobject, params, unity_instance?}
HTTP->>Hub: PluginHub.get_sessions()
Hub-->>HTTP: sessions list
HTTP->>Hub: send_command(session_id, command_type, params)
Hub-->>HTTP: result JSON
HTTP-->>Conn: JSONResponse(result)
Conn-->>GameobjectCmd: result dict
GameobjectCmd->>Conn: run_command("manage_components", add Rigidbody, config) (optional)
GameobjectCmd->>Conn: run_command("manage_components", add BoxCollider, config) (optional)
GameobjectCmd->>CLI: formatted output via format_output
CLI-->>User: text/json/table result
Class diagram for CLI configuration and utility modulesclassDiagram
class CLIConfig {
+str host
+int port
+int timeout
+str format
+str unity_instance
+from_env() CLIConfig
}
class Context {
+CLIConfig config
+bool verbose
+__init__()
}
class ConfigModule {
+CLIConfig _config
+get_config() CLIConfig
+set_config(config CLIConfig) void
}
class ConnectionModule {
+send_command(command_type str, params dict, config CLIConfig, timeout int) dict
+run_command(command_type str, params dict, config CLIConfig, timeout int) dict
+check_connection(config CLIConfig) bool
+run_check_connection(config CLIConfig) bool
+list_unity_instances(config CLIConfig) dict
+run_list_instances(config CLIConfig) dict
+warn_if_remote_host(config CLIConfig) void
}
class UnityConnectionError {
+UnityConnectionError(message str)
}
class OutputModule {
+format_output(data any, format_type str) str
+format_as_json(data any) str
+format_as_text(data any, indent int) str
+format_as_table(data any) str
+print_success(message str) void
+print_error(message str) void
+print_warning(message str) void
+print_info(message str) void
}
class MainCLI {
+cli(host str, port int, timeout int, format str, instance str, verbose bool) void
+status() void
+instances() void
+raw_command(command_type str, params str) void
+register_commands() void
+main() void
}
class GameobjectCommands {
+gameobject()
+find(search_term str, method str, include_inactive bool, limit int, cursor int)
+create(name str, primitive str, position tuple, rotation tuple, scale tuple, parent str, tag str, layer str, components str, save_prefab bool, prefab_path str)
+modify(target str, name str, position tuple, rotation tuple, scale tuple, parent str, tag str, layer str, active bool, add_components str, remove_components str, search_method str)
+delete(target str, search_method str, force bool)
+duplicate(target str, name str, offset tuple, search_method str)
+move(target str, reference str, direction str, distance float, local bool, search_method str)
}
MainCLI --> Context : uses
Context --> CLIConfig : holds
MainCLI --> ConfigModule : uses
MainCLI --> ConnectionModule : uses
MainCLI --> OutputModule : uses
ConfigModule --> CLIConfig : manages
ConnectionModule --> CLIConfig : reads
ConnectionModule --> UnityConnectionError : raises
OutputModule ..> CLIConfig : uses format field
GameobjectCommands --> ConfigModule : get_config
GameobjectCommands --> ConnectionModule : run_command
GameobjectCommands --> OutputModule : format_output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds a full Click-based CLI (entrypoint, many command groups), CLI utilities (config, connection, output), server endpoints to forward/list CLI commands, comprehensive CLI docs, and an extensive pytest suite for the CLI surface and connection logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as Click CLI (cli.main)
participant Config as CLIConfig
participant Conn as Connection Utils
participant Server as MCP Server (/api/command)
participant Unity as Unity Instance
User->>CLI: run command (e.g. gameobject create ...)
CLI->>Config: get_config()
Config-->>CLI: CLIConfig
CLI->>Conn: run_command(type, params, config)
Conn->>Server: POST /api/command (JSON)
Server->>Server: select Unity instance (by unity_instance or default)
Server->>Unity: forward command payload
Unity-->>Server: command result
Server-->>Conn: JSON response
Conn-->>CLI: result dict
CLI->>CLI: format_output(result, format)
CLI->>User: formatted output / success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 9 issues, and left some high level feedback:
- The
/api/commandroute silently falls back to the first available session whenunity_instanceis missing or doesn’t match, which can be surprising in multi-instance setups—consider returning a 400/409 with a clear error when there’s an explicit but unresolvedunity_instance, and/or requiring disambiguation when multiple candidates exist. - Several commands inspect responses with ad‑hoc checks like
result.get("success") or result.get("data") or result.get("result"); it would be more robust to centralize response normalization/validation (e.g., a helper that extractssuccess,dataand standardizes error reporting) so all commands interpret server responses consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `/api/command` route silently falls back to the first available session when `unity_instance` is missing or doesn’t match, which can be surprising in multi-instance setups—consider returning a 400/409 with a clear error when there’s an explicit but unresolved `unity_instance`, and/or requiring disambiguation when multiple candidates exist.
- Several commands inspect responses with ad‑hoc checks like `result.get("success") or result.get("data") or result.get("result")`; it would be more robust to centralize response normalization/validation (e.g., a helper that extracts `success`, `data` and standardizes error reporting) so all commands interpret server responses consistently.
## Individual Comments
### Comment 1
<location> `Server/src/main.py:328-330` </location>
<code_context>
+ session_id = sid
+ break
+
+ if not session_id:
+ # Use first available session
+ session_id = next(iter(sessions.sessions.keys()))
+
+ # Send command to Unity
</code_context>
<issue_to_address>
**issue (bug_risk):** Using the first available session when a specific `unity_instance` is provided but not found may be surprising behavior.
If `unity_instance` is provided but not found, silently falling back to the first session can cause commands to be sent to the wrong Unity instance, and the CLI has no easy way to detect this. It would be safer to return an explicit error in that case (e.g., `"Unity instance '<value>' not found"`), or otherwise handle "not provided" vs. "provided but missing" differently rather than auto-selecting.
</issue_to_address>
### Comment 2
<location> `Server/src/cli/commands/material.py:92-98` </location>
<code_context>
[email protected]("r", type=float)
[email protected]("g", type=float)
[email protected]("b", type=float)
[email protected]("a", type=float, default=1.0)
[email protected](
+ "--property", "-p",
+ default="_Color",
+ help="Color property name (default: _Color)."
+)
+def set_color(path: str, r: float, g: float, b: float, a: float, property: str):
+ """Set a material's color.
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a default on a required Click argument can lead to confusing or unsupported behavior.
Here `a` is a positional argument but also has `default=1.0`. In Click, defaults are usually used with `required=False` and options rather than required positional arguments, and behavior can vary between Click versions. Consider either making `a` an option (e.g., `--alpha`) with a default, or removing the default and requiring all four components explicitly.
</issue_to_address>
### Comment 3
<location> `Server/src/cli/commands/ui.py:91` </location>
<code_context>
+ default=(0, 0),
+ help="Anchored position X Y."
+)
+def create_text(name: str, parent: str, text: str, position: tuple):
+ """Create a UI Text element (TextMeshPro).
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `position` parameter for `create-text` is currently unused.
The CLI accepts `--position` and passes it to `create_text`, but the function never uses it (e.g., on a `RectTransform` anchored position). This makes the option misleading. Either apply the position (possibly via a `manage_components`/`manage_gameobject` call) or remove the option until it’s supported.
</issue_to_address>
### Comment 4
<location> `Server/src/cli/commands/audio.py:20-23` </location>
<code_context>
[email protected]("play")
[email protected]("target")
[email protected]("state_name")
[email protected](
+ "--layer", "-l",
+ default=0,
</code_context>
<issue_to_address>
**issue (bug_risk):** The `--clip` option in `audio play` is accepted but never used.
The `play` command takes a `clip` parameter but only toggles an `AudioSource` property and never sends the clip path to Unity. If clip selection isn’t supported yet, either remove this option or pass it through to `manage_components` (or equivalent) so the backend can actually use it.
</issue_to_address>
### Comment 5
<location> `Server/tests/test_cli.py:199-208` </location>
<code_context>
+ result = await check_connection()
+ assert result is False
+
+ @pytest.mark.asyncio
+ async def test_send_command_success(self, mock_unity_response):
+ """Test successful command sending."""
+ mock_response = MagicMock()
+ mock_response.status_code = 200
+ mock_response.json.return_value = mock_unity_response
+
+ with patch("httpx.AsyncClient") as mock_client:
+ mock_client.return_value.__aenter__.return_value.post = AsyncMock(
+ return_value=mock_response
+ )
+ mock_response.raise_for_status = MagicMock()
+
+ result = await send_command("test_command", {"param": "value"})
+ assert result == mock_unity_response
+
+ @pytest.mark.asyncio
+ async def test_send_command_connection_error(self):
+ """Test command sending with connection error."""
+ with patch("httpx.AsyncClient") as mock_client:
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the specific httpx error branches in send_command
Current tests only exercise the happy path and a generic `Exception` from `send_command`, but the code has specific handling for `httpx.ConnectError`, `httpx.TimeoutException`, and `httpx.HTTPStatusError` with distinct messages. Please add tests that mock `httpx.AsyncClient.post` to raise each of these exceptions and assert that `UnityConnectionError` is raised with the correct message fragment, so the detailed error handling is verified and doesn't regress.
Suggested implementation:
```python
# Connection Tests
# =============================================================================
class TestConnection:
=======
assert "key" in table_result.lower() or "Key" in table_result
@pytest.mark.asyncio
async def test_send_command_httpx_connect_error():
"""send_command should wrap httpx.ConnectError in UnityConnectionError with a connection-related message."""
# Simulate a low-level connection error from httpx
with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.post = AsyncMock(
side_effect=httpx.ConnectError("Unable to connect")
)
with pytest.raises(UnityConnectionError) as exc_info:
await send_command("test_command", {"param": "value"})
message = str(exc_info.value).lower()
assert "connect" in message or "connection" in message
@pytest.mark.asyncio
async def test_send_command_httpx_timeout_error():
"""send_command should wrap httpx.TimeoutException in UnityConnectionError with a timeout-related message."""
with patch("httpx.AsyncClient") as mock_client:
mock_client.return_value.__aenter__.return_value.post = AsyncMock(
side_effect=httpx.TimeoutException("Request timed out")
)
with pytest.raises(UnityConnectionError) as exc_info:
await send_command("test_command", {"param": "value"})
message = str(exc_info.value).lower()
assert "timeout" in message or "timed out" in message
@pytest.mark.asyncio
async def test_send_command_httpx_http_status_error():
"""send_command should wrap httpx.HTTPStatusError in UnityConnectionError with an HTTP-status-related message."""
# Create a Response and Request to satisfy HTTPStatusError requirements
request = httpx.Request("POST", "http://unity-endpoint")
response = httpx.Response(status_code=500, request=request)
http_status_error = httpx.HTTPStatusError(
"Unity returned an error status",
request=request,
response=response,
)
with patch("httpx.AsyncClient") as mock_client:
# send_command is expected to call response.raise_for_status(), which will raise HTTPStatusError
mock_response = MagicMock()
mock_response.raise_for_status.side_effect = http_status_error
mock_client.return_value.__aenter__.return_value.post = AsyncMock(
return_value=mock_response
)
with pytest.raises(UnityConnectionError) as exc_info:
await send_command("test_command", {"param": "value"})
message = str(exc_info.value).lower()
assert "http" in message or "status" in message or "error response" in message
# =============================================================================
# Connection Tests
# =============================================================================
class TestConnection:
```
These tests assume:
1. `pytest`, `httpx`, `MagicMock`, `AsyncMock`, `patch`, `UnityConnectionError`, and `send_command` are already imported in `Server/tests/test_cli.py`. If any are missing, add appropriate imports at the top of the file.
2. The error messages raised by `send_command` for `httpx.ConnectError`, `httpx.TimeoutException`, and `httpx.HTTPStatusError` contain connection-, timeout-, and HTTP/status-related words respectively. If your actual messages differ, adjust the `assert` conditions in the tests to match the real message fragments (for example, assert for `"failed to connect to unity"` instead of the generic `"connect"` / `"connection"`).
</issue_to_address>
### Comment 6
<location> `Server/tests/test_cli.py:735-744` </location>
<code_context>
+class TestGlobalOptions:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that asserts warn_if_remote_host is triggered for non-local hosts
Current global option tests cover `--host`, `--port`, `--timeout`, and `--format`, but don’t check that `warn_if_remote_host` is invoked for non-local hosts. Please add a CLI test (e.g., `unity-mcp --host 10.0.0.5 status`) that captures stderr/stdout to assert the warning text appears and that the command still returns a valid status. This will help prevent regressions in the remote-host safety warning wiring.
Suggested implementation:
```python
class TestGlobalOptions:
"""Tests for global CLI options."""
def test_custom_host(self, runner, mock_unity_response):
"""Test custom host option."""
with patch("cli.main.run_check_connection", return_value=True):
with patch("cli.main.run_list_instances", return_value={"instances": []}):
result = runner.invoke(cli, ["--host", "192.168.1.100", "status"])
assert result.exit_code == 0
def test_remote_host_warns_and_succeeds(self, runner, mock_unity_response):
"""Test that a non-local host triggers a warning but the command still succeeds."""
with patch("cli.main.run_check_connection", return_value=True):
with patch("cli.main.run_list_instances", return_value={"instances": []}):
with patch("cli.main.warn_if_remote_host") as mock_warn_if_remote_host:
# Emit a known warning string to stderr when the remote host check runs
mock_warn_if_remote_host.side_effect = lambda host: click.echo(
"REMOTE HOST SAFETY WARNING", err=True
)
result = runner.invoke(cli, ["--host", "10.0.0.5", "status"])
# Command should still complete successfully
assert result.exit_code == 0
# Ensure the remote host warning hook was invoked with the non-local host
mock_warn_if_remote_host.assert_called_once_with("10.0.0.5")
# Ensure the warning text is visible in the CLI output (stderr captured by Click test runner)
assert "REMOTE HOST SAFETY WARNING" in result.output
def test_custom_port(self, runner, mock_unity_response):
```
1. Ensure `click` is imported at the top of `Server/tests/test_cli.py`, for example:
- Add `import click` alongside the other imports if it is not already present.
2. If the real `warn_if_remote_host` signature differs (e.g., additional parameters or different module path), adjust the patch target (`"cli.main.warn_if_remote_host"`) and `assert_called_once_with(...)` arguments accordingly.
3. If your test suite differentiates between `stdout` and `stderr` using a different mechanism than Click's `CliRunner`, adapt the `result.output` assertion to match your existing pattern (e.g., `result.stderr` if available).
</issue_to_address>
### Comment 7
<location> `Server/tests/test_cli.py:285-291` </location>
<code_context>
+ result = runner.invoke(cli, ["--version"])
+ assert result.exit_code == 0
+
+ def test_status_connected(self, runner, mock_instances_response):
+ """Test status command when connected."""
+ with patch("cli.main.run_check_connection", return_value=True):
+ with patch("cli.main.run_list_instances", return_value=mock_instances_response):
+ result = runner.invoke(cli, ["status"])
+ assert result.exit_code == 0
+ assert "Connected" in result.output
+
+ def test_status_disconnected(self, runner):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a status test for when listing instances fails with UnityConnectionError
There’s an untested path where `run_check_connection` returns `True` but `run_list_instances` raises `UnityConnectionError`; in that case the CLI should print an informational message and exit cleanly. Please add a test that patches `run_check_connection` to return `True` and `run_list_instances` to raise `UnityConnectionError`, then assert exit code 0 and that the expected "Could not retrieve Unity instances" (or equivalent) message is printed.
Suggested implementation:
```python
def test_status_connected(self, runner, mock_instances_response):
"""Test status command when connected."""
with patch("cli.main.run_check_connection", return_value=True):
with patch("cli.main.run_list_instances", return_value=mock_instances_response):
result = runner.invoke(cli, ["status"])
assert result.exit_code == 0
assert "Connected" in result.output
def test_status_instances_error(self, runner):
"""Test status command when listing instances fails."""
with patch("cli.main.run_check_connection", return_value=True):
with patch("cli.main.run_list_instances", side_effect=UnityConnectionError("test error")):
result = runner.invoke(cli, ["status"])
assert result.exit_code == 0
assert "Could not retrieve Unity instances" in result.output
def test_status_disconnected(self, runner):
"""Test status command when disconnected."""
with patch("cli.main.run_check_connection", return_value=False):
result = runner.invoke(cli, ["status"])
assert result.exit_code == 1
assert "Cannot connect" in result.output
```
1. Ensure `UnityConnectionError` is imported at the top of `Server/tests/test_cli.py`, for example:
`from cli.main import UnityConnectionError` or from the module where it is defined (e.g. `from cli.exceptions import UnityConnectionError`), matching the existing codebase.
2. Confirm the exact message printed by the CLI when instance listing fails; if it differs from `"Could not retrieve Unity instances"`, update the assertion string in `test_status_instances_error` accordingly.
</issue_to_address>
### Comment 8
<location> `Server/tests/test_cli.py:306-277` </location>
<code_context>
+ result = runner.invoke(cli, ["instances"])
+ assert result.exit_code == 0
+
+ def test_raw_command(self, runner, mock_unity_response):
+ """Test raw command."""
+ with patch("cli.main.run_command", return_value=mock_unity_response):
+ result = runner.invoke(cli, ["raw", "test_command", '{"param": "value"}'])
+ assert result.exit_code == 0
+
+ def test_raw_command_invalid_json(self, runner):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a raw command test that covers UnityConnectionError handling
Currently we only test the success and invalid-JSON paths. Since `cli.main.raw_command` should treat a `UnityConnectionError` from `run_command` as an error (message + exit code 1), please add a test that patches `cli.main.run_command` to raise `UnityConnectionError("some error")`, then asserts `runner.invoke` returns `exit_code == 1` and that the error message appears in the output/stderr.
Suggested implementation:
```python
# GameObject Command Tests
# =============================================================================
=======
def test_raw_command_invalid_json(self, runner):
"""Test raw command with invalid JSON."""
result = runner.invoke(cli, ["raw", "test_command", "invalid json"])
assert result.exit_code == 1
assert "Invalid JSON" in result.output
def test_raw_command_unity_connection_error(self, runner):
"""Test raw command handling UnityConnectionError from run_command."""
with patch("cli.main.run_command", side_effect=UnityConnectionError("some error")):
result = runner.invoke(cli, ["raw", "test_command", '{"param": "value"}'])
assert result.exit_code == 1
assert "some error" in result.output
# =============================================================================
# GameObject Command Tests
# =============================================================================
```
If `UnityConnectionError` is not already imported in `Server/tests/test_cli.py`, add an import at the top of the file, for example:
- If it lives in `cli.exceptions`:
```python
from cli.exceptions import UnityConnectionError
```
or match whatever module is used elsewhere in this file for other `UnityConnectionError`-related tests.
</issue_to_address>
### Comment 9
<location> `Server/tests/test_cli.py:126-135` </location>
<code_context>
+class TestOutputFormatting:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for format_as_text on successful responses that still include success/message fields
`format_as_text` has special handling for dicts with `success` and `data`/`result` fields where it recurses into the inner payload. Please add a test for an input like `{"success": True, "message": "OK", "data": {"foo": "bar"}}` that asserts only the inner `data` is rendered and `success`/`message` are omitted, to lock in this meta-field stripping behavior.
Suggested implementation:
```python
class TestOutputFormatting:
"""Tests for output formatting utilities."""
def test_format_as_json(self):
"""Test JSON formatting."""
data = {"key": "value", "number": 42}
result = format_as_json(data)
parsed = json.loads(result)
assert parsed == data
def test_format_as_text_strips_meta_fields_on_success(self):
"""format_as_text should render only inner data for success-wrapped payloads."""
payload = {"success": True, "message": "OK", "data": {"foo": "bar"}}
rendered = format_as_text(payload)
# Only the inner data payload should be rendered; meta fields are stripped
assert "foo" in rendered
assert "bar" in rendered
assert "success" not in rendered
assert "message" not in rendered
def test_format_as_json_with_complex_types(self):
```
If `format_as_text` is not yet imported in this test module, you will also need to add an import at the top of `Server/tests/test_cli.py`, matching how `format_as_json` is imported (for example, `from Server.cli import format_as_json, format_as_text` or similar, depending on the existing import style).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if not session_id: | ||
| # Use first available session | ||
| session_id = next(iter(sessions.sessions.keys())) |
There was a problem hiding this comment.
issue (bug_risk): Using the first available session when a specific unity_instance is provided but not found may be surprising behavior.
If unity_instance is provided but not found, silently falling back to the first session can cause commands to be sent to the wrong Unity instance, and the CLI has no easy way to detect this. It would be safer to return an explicit error in that case (e.g., "Unity instance '<value>' not found"), or otherwise handle "not provided" vs. "provided but missing" differently rather than auto-selecting.
Server/src/cli/commands/material.py
Outdated
| @click.argument("a", type=float, default=1.0) | ||
| @click.option( | ||
| "--property", "-p", | ||
| default="_Color", | ||
| help="Color property name (default: _Color)." | ||
| ) | ||
| def set_color(path: str, r: float, g: float, b: float, a: float, property: str): |
There was a problem hiding this comment.
issue (bug_risk): Using a default on a required Click argument can lead to confusing or unsupported behavior.
Here a is a positional argument but also has default=1.0. In Click, defaults are usually used with required=False and options rather than required positional arguments, and behavior can vary between Click versions. Consider either making a an option (e.g., --alpha) with a default, or removing the default and requiring all four components explicitly.
| class TestOutputFormatting: | ||
| """Tests for output formatting utilities.""" | ||
|
|
||
| def test_format_as_json(self): | ||
| """Test JSON formatting.""" | ||
| data = {"key": "value", "number": 42} | ||
| result = format_as_json(data) | ||
| parsed = json.loads(result) | ||
| assert parsed == data | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add a test for format_as_text on successful responses that still include success/message fields
format_as_text has special handling for dicts with success and data/result fields where it recurses into the inner payload. Please add a test for an input like {"success": True, "message": "OK", "data": {"foo": "bar"}} that asserts only the inner data is rendered and success/message are omitted, to lock in this meta-field stripping behavior.
Suggested implementation:
class TestOutputFormatting:
"""Tests for output formatting utilities."""
def test_format_as_json(self):
"""Test JSON formatting."""
data = {"key": "value", "number": 42}
result = format_as_json(data)
parsed = json.loads(result)
assert parsed == data
def test_format_as_text_strips_meta_fields_on_success(self):
"""format_as_text should render only inner data for success-wrapped payloads."""
payload = {"success": True, "message": "OK", "data": {"foo": "bar"}}
rendered = format_as_text(payload)
# Only the inner data payload should be rendered; meta fields are stripped
assert "foo" in rendered
assert "bar" in rendered
assert "success" not in rendered
assert "message" not in rendered
def test_format_as_json_with_complex_types(self):If format_as_text is not yet imported in this test module, you will also need to add an import at the top of Server/tests/test_cli.py, matching how format_as_json is imported (for example, from Server.cli import format_as_json, format_as_text or similar, depending on the existing import style).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @Server/src/cli/commands/animation.py:
- Around line 33-60: The play function accepts a layer parameter but never sends
it to Unity; update the params dict in play to include the layer (e.g.,
params["layer"] = layer) so the "manage_components" call receives the layer
value, or if layer support is not yet intended remove the layer parameter from
play's signature or add a clear TODO comment and validation; locate the play
function and modify the params construction (or signature) accordingly to ensure
the parameter is used or intentionally omitted.
- Line 48: The PR is trying to set "Play" as a property but Animator.Play is a
method; update the code that emits the component change (the entry with
"property": "Play") to either (A) call a method-invocation path in the
manage_components backend (implement method invocation support there and handle
calling Animator.Play(...)) or (B) replace this manage_components call with an
animation-specific tool (similar to ManageVFX.cs) that invokes Animator.Play on
the target component; locate the emitter that writes the "property": "Play"
entry and change it to emit a method-invoke request (or swap to the custom
animation tool) and wire up any required parameters for Animator.Play
accordingly.
In @Server/src/cli/commands/audio.py:
- Around line 18-57: The play command accepts a --clip option but never uses it;
update the play function so that when clip is not None you add it to the params
payload (e.g., params["clip"] = clip) before calling
run_command("manage_components", params, config); if specifying a clip is not a
supported use case, remove the --clip Click option and the clip parameter from
play instead.
In @Server/src/cli/commands/code.py:
- Line 1: The module docstring claims "search and read source code" but only the
read command is implemented; update the top-level docstring in
Server/src/cli/commands/code.py to accurately reflect implemented functionality
(e.g., "Read source code" or "Read source code (search not yet implemented)") or
add a clear TODO note about implementing the search command, and mention the
implemented symbol 'read' so reviewers can verify consistency.
In @Server/src/cli/commands/gameobject.py:
- Around line 181-189: The component addition loop currently calls
run_command("manage_components", ...) for each entry in component_list without
per-item error handling; wrap each run_command call in a try/except (or check
its return value) inside the for-loop that iterates component_list, log or
collect failures (including component name and error/response) and continue to
the next component, and after the loop report any collected failures (or
raise/return a consolidated error) so callers can see which components failed;
references: components, result, component_list, run_command,
"manage_components", name, config.
In @Server/src/cli/commands/lighting.py:
- Around line 66-68: The current guard incorrectly uses "or" across fields so a
falsy success with truthy data/result lets execution continue; update the check
on create_result to only verify success (or success and data) — e.g., replace
the condition with if not create_result.get("success") (or if not
(create_result.get("success") and create_result.get("data"))) before calling
click.echo(format_output(create_result, config.format)) and returning so the
command aborts when creation failed.
In @Server/src/cli/commands/material.py:
- Around line 171-176: The docstring/example uses an invalid value "by_id" for
the --search-method Click option; update the example(s) to use one of the
allowed choices ("by_name", "by_path", "by_tag", "by_layer", "by_component") or
alternatively add "by_id" to the Click option choices array on the
--search-method declaration so they match; ensure you change all occurrences
(including the example at the other noted location) so the example and the
click.option choices remain consistent.
In @Server/src/cli/commands/script.py:
- Around line 147-150: The params construction is using fragile string-splitting
on path; update the logic that builds params (the "params" dict in the delete
command) to use robust path utilities instead of path.split: derive the script
name with pathlib.Path(path).stem (or
os.path.splitext(os.path.basename(path))[0]) and derive the parent with
pathlib.Path(path).parent.as_posix() (or os.path.dirname(path)), falling back to
"Assets" when parent is empty or "."; then assign those values into the existing
params dict ("action": "delete", "name", "path") so behavior matches the safer
parsing used in the read command.
In @Server/src/cli/commands/ui.py:
- Around line 190-228: The --sprite option is accepted by create_image but never
applied; after the existing run_command call that adds the Image component in
create_image, if sprite is not None call run_command again to set the Image
component's sprite property on the created GameObject (use the same config and
target name), e.g. invoke the manage_components command to set the Image
component's "sprite" (or include it in the add call as a properties/payload
field) so the provided sprite path is applied to the component.
- Around line 84-132: create_text accepts a position tuple but never applies it
to the RectTransform; after adding the TextMeshProUGUI component (in
create_text) call run_command("manage_components", { "action": "set_property",
"target": name, "componentType": "RectTransform", "property":
"anchoredPosition", "value": list(position) }, config) (or use
"anchoredPosition3D"/"localPosition" if your backend expects 3D) and ensure you
pass the position variable (converted to the expected list/format) so the
RectTransform receives the anchored position.
In @Server/src/cli/utils/config.py:
- Around line 18-27: The from_env classmethod on CLIConfig currently casts
UNITY_MCP_HTTP_PORT and UNITY_MCP_TIMEOUT to int without validation, which can
raise ValueError; wrap the int conversions in validation/error handling inside
CLIConfig.from_env (catch ValueError/TypeError), provide a clear error message
including the env var name and invalid value (and optionally fallback to
defaults), and re-raise or exit gracefully so the app doesn’t crash with an
opaque traceback; reference the symbols CLIConfig.from_env, UNITY_MCP_HTTP_PORT,
and UNITY_MCP_TIMEOUT when locating and updating the code.
🧹 Nitpick comments (19)
Server/src/cli/utils/connection.py (2)
80-96: Consider adding exception chaining for better debugging.The exception handlers re-raise errors without preserving the exception chain. Adding
from e(orfrom Nonewhere deliberate) improves debugging by preserving the original traceback.♻️ Proposed fix to add exception chaining
except httpx.ConnectError as e: raise UnityConnectionError( f"Cannot connect to Unity MCP server at {cfg.host}:{cfg.port}. " f"Make sure the server is running and Unity is connected.\n" f"Error: {e}" - ) + ) from e except httpx.TimeoutException: raise UnityConnectionError( f"Connection to Unity timed out after {timeout or cfg.timeout}s. " f"Unity may be busy or unresponsive." - ) + ) from None except httpx.HTTPStatusError as e: raise UnityConnectionError( f"HTTP error from server: {e.response.status_code} - {e.response.text}" - ) + ) from e except Exception as e: - raise UnityConnectionError(f"Unexpected error: {e}") + raise UnityConnectionError(f"Unexpected error: {e}") from e
161-183: Consider logging endpoint failures for easier debugging.The try-except-continue pattern silently swallows errors when attempting each endpoint. If both endpoints fail, users only see the generic "No working endpoint found" message without knowing why each endpoint failed. Adding debug logging would aid troubleshooting.
💡 Suggested improvement with logging
+ import logging + logger = logging.getLogger(__name__) + async with httpx.AsyncClient() as client: for url in urls_to_try: try: response = await client.get(url, timeout=10) if response.status_code == 200: data = response.json() # Normalize response format if "instances" in data: return data elif "sessions" in data: # Convert sessions format to instances format instances = [] for session_id, details in data["sessions"].items(): instances.append({ "session_id": session_id, "project": details.get("project", "Unknown"), "hash": details.get("hash", ""), "unity_version": details.get("unity_version", "Unknown"), "connected_at": details.get("connected_at", ""), }) return {"success": True, "instances": instances} + else: + logger.debug(f"Endpoint {url} returned status {response.status_code}") - except Exception: + except Exception as e: + logger.debug(f"Failed to connect to {url}: {e}") continueServer/src/cli/commands/code.py (1)
44-44: Consider broader file type support.The command strips
.csextensions, assuming C# files. However, the command group is namedcode(notscript), which suggests it might be intended for broader source file reading. Consider whether this should support other file types or if the naming should be clarified.Server/src/main.py (2)
336-338: Uselogging.exceptionto preserve traceback.Line 337 uses
logger.error, which discards the exception traceback. For debugging production issues, uselogger.exceptioninstead to preserve the full stack trace.🔍 Suggested fix
except Exception as e: - logger.error(f"CLI command error: {e}") + logger.exception(f"CLI command error: {e}") return JSONResponse({"success": False, "error": str(e)}, status_code=500)Based on static analysis hints.
356-357: Add error logging for consistency.The
cli_instances_routeexception handler doesn't log errors, unlikecli_command_routeat line 337. For operational visibility and debugging, consider logging the exception here as well.📊 Suggested addition
except Exception as e: + logger.exception("CLI instances query error") return JSONResponse({"success": False, "error": str(e)}, status_code=500)Server/src/cli/utils/config.py (1)
25-25: Validate format value.Line 25 accepts any string for the
formatfield, but based on the codebase context (output.py uses "text", "json", "table"), only specific values are valid. Consider validating the format value and falling back to "text" or raising an error for invalid values.✨ Suggested validation
+ format_value = os.environ.get("UNITY_MCP_FORMAT", "text") + if format_value not in ("text", "json", "table"): + raise ValueError(f"Invalid UNITY_MCP_FORMAT: {format_value}. Must be one of: text, json, table") + return cls( host=os.environ.get("UNITY_MCP_HOST", "127.0.0.1"), port=int(os.environ.get("UNITY_MCP_HTTP_PORT", "8080")), timeout=int(os.environ.get("UNITY_MCP_TIMEOUT", "30")), - format=os.environ.get("UNITY_MCP_FORMAT", "text"), + format=format_value, unity_instance=os.environ.get("UNITY_MCP_INSTANCE"), )Server/src/cli/commands/animation.py (1)
63-85: Placeholder command doesn't execute.The
set-parametercommand is a non-functional placeholder that only prints informational messages (lines 84-85) without executing any Unity operations. While this may be intentional per the PR description, consider either:
- Implementing the functionality
- Adding a clear warning in the help text that this is not yet implemented
- Hiding the command until implementation is complete
📋 Suggested help text clarification
def set_parameter(target: str, param_name: str, value: str, param_type: str): - """Set an Animator parameter. + """Set an Animator parameter (placeholder - not yet implemented). \b Examples:Server/tests/test_cli.py (1)
735-770: Remove unusedmock_unity_responsefixture arguments.The
mock_unity_responsefixture is declared but never used intest_custom_host,test_custom_port, andtest_timeout_option. These tests mockrun_check_connectionandrun_list_instancesdirectly, so the fixture is unnecessary.♻️ Proposed fix
- def test_custom_host(self, runner, mock_unity_response): + def test_custom_host(self, runner): """Test custom host option.""" with patch("cli.main.run_check_connection", return_value=True): with patch("cli.main.run_list_instances", return_value={"instances": []}): result = runner.invoke(cli, ["--host", "192.168.1.100", "status"]) assert result.exit_code == 0 - def test_custom_port(self, runner, mock_unity_response): + def test_custom_port(self, runner): """Test custom port option.""" with patch("cli.main.run_check_connection", return_value=True): with patch("cli.main.run_list_instances", return_value={"instances": []}): result = runner.invoke(cli, ["--port", "9090", "status"]) assert result.exit_code == 0 ... - def test_timeout_option(self, runner, mock_unity_response): + def test_timeout_option(self, runner): """Test timeout option.""" with patch("cli.main.run_check_connection", return_value=True): with patch("cli.main.run_list_instances", return_value={"instances": []}): result = runner.invoke(cli, ["--timeout", "60", "status"]) assert result.exit_code == 0Server/src/cli/commands/audio.py (1)
96-130: Consider adding volume range validation.Unity's
AudioSource.volumeis clamped between 0.0 and 1.0. Adding client-side validation would provide clearer feedback to users.♻️ Suggested validation
def volume(target: str, level: float, search_method: Optional[str]): """Set audio volume on a target's AudioSource. ... """ config = get_config() + + if not 0.0 <= level <= 1.0: + print_error("Volume level must be between 0.0 and 1.0") + sys.exit(1) params: dict[str, Any] = {Server/src/cli/commands/asset.py (1)
246-249: Moveosimport to module level.The
osimport is inside the function, which is inconsistent with the module's import style. Move it to the top with other imports.♻️ Proposed fix
"""Asset CLI commands.""" import sys +import os import json import click from typing import Optional, AnyThen remove line 247:
- import os dir_path = os.path.dirname(path)Server/src/cli/commands/prefab.py (1)
20-24: Consider constraining mode values withclick.Choice.The mode option accepts any string, but Unity's PrefabStage only supports specific modes. Using
click.Choicewould provide better validation and help text.♻️ Suggested improvement
@click.option( "--mode", "-m", - default="InIsolation", - help="Prefab stage mode (InIsolation)." + type=click.Choice(["Normal", "InIsolation", "InContext"]), + default="InIsolation", + help="Prefab stage mode." )Server/src/cli/commands/ui.py (1)
43-45: Overly permissive success check may mask failures.The condition
result.get("success") or result.get("data") or result.get("result")could treat an error response as successful if it containsdataorresultkeys. Consider using onlyresult.get("success")for consistency with other command modules.♻️ Proposed fix
- if not (result.get("success") or result.get("data") or result.get("result")): + if not result.get("success"): click.echo(format_output(result, config.format)) returnApply this pattern to all occurrences in this file (lines 43, 108, 164, 213).
Server/src/cli/commands/scene.py (1)
224-245: Consider validating supersize range.The help text indicates supersize should be 1-4, but no validation enforces this. Adding client-side validation would provide clearer feedback.
♻️ Suggested validation
def screenshot(filename: Optional[str], supersize: int): """Capture a screenshot of the scene. ... """ config = get_config() + + if not 1 <= supersize <= 4: + print_error("Supersize must be between 1 and 4") + sys.exit(1) params: dict[str, Any] = {"action": "screenshot"}Server/src/cli/main.py (2)
31-36: Short option-hconflicts with Click's default--help.Using
-hfor--hostconflicts with the conventional-h/--helpshortcut that many CLI users expect. Click doesn't add-hby default, but this could cause confusion.Consider using a different short option
@click.option( - "--host", "-h", + "--host", "-H", default="127.0.0.1", envvar="UNITY_MCP_HOST", help="MCP server host address." )
180-260: Repetitive command registration can be simplified with a loop.The
register_commandsfunction repeats the same try/except pattern 13 times. This could be simplified using a loop, reducing maintenance burden and improving readability.♻️ Suggested refactor using a loop
def register_commands(): """Register all command groups.""" - try: - from cli.commands.gameobject import gameobject - cli.add_command(gameobject) - except ImportError: - pass - - try: - from cli.commands.component import component - cli.add_command(component) - except ImportError: - pass - - # ... (remaining repetitive blocks) + command_modules = [ + ("gameobject", "gameobject"), + ("component", "component"), + ("scene", "scene"), + ("asset", "asset"), + ("script", "script"), + ("code", "code"), + ("editor", "editor"), + ("prefab", "prefab"), + ("material", "material"), + ("lighting", "lighting"), + ("animation", "animation"), + ("audio", "audio"), + ("ui", "ui"), + ] + + for module_name, command_name in command_modules: + try: + module = __import__(f"cli.commands.{module_name}", fromlist=[command_name]) + cli.add_command(getattr(module, command_name)) + except ImportError: + passServer/src/cli/commands/material.py (1)
6-6: Unused importTuple.
Tupleis imported but not used in this module.Remove unused import
-from typing import Optional, Any, Tuple +from typing import Optional, AnyServer/src/cli/commands/gameobject.py (1)
291-294: Inconsistent type annotations onparamsdictionaries.The
paramsdictionary inmodify,delete,duplicate, andmovecommands lacks type annotations, unlikefindandcreatecommands which usedict[str, Any].Add consistent type annotations
- params = { + params: dict[str, Any] = { "action": "modify", "target": target, }Apply similar change to
delete,duplicate, andmovecommands.Also applies to: 354-357, 408-411, 478-485
Server/src/cli/utils/output.py (1)
5-5: Unused importUnion.
Unionis imported but not used in the module.Remove unused import
-from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, OptionalServer/src/cli/commands/editor.py (1)
8-8: Unused importprint_info.
print_infois imported but not used in this module.Remove unused import
-from cli.utils.output import format_output, print_error, print_success, print_info +from cli.utils.output import format_output, print_error, print_success
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Server/pyproject.tomlServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/__init__.pyServer/src/cli/commands/__init__.pyServer/src/cli/commands/animation.pyServer/src/cli/commands/asset.pyServer/src/cli/commands/audio.pyServer/src/cli/commands/code.pyServer/src/cli/commands/component.pyServer/src/cli/commands/editor.pyServer/src/cli/commands/gameobject.pyServer/src/cli/commands/lighting.pyServer/src/cli/commands/material.pyServer/src/cli/commands/prefab.pyServer/src/cli/commands/scene.pyServer/src/cli/commands/script.pyServer/src/cli/commands/ui.pyServer/src/cli/main.pyServer/src/cli/utils/__init__.pyServer/src/cli/utils/config.pyServer/src/cli/utils/connection.pyServer/src/cli/utils/output.pyServer/src/main.pyServer/tests/test_cli.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-12-29T04:54:17.743Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 490
File: Server/pyproject.toml:33-33
Timestamp: 2025-12-29T04:54:17.743Z
Learning: Pin the fastmcp dependency to an exact version in Server/pyproject.toml (e.g., exact string 2.14.1). Avoid range pins like >=2.13.0 to prevent breaking changes affecting MCP tools. Apply the same exact-version pinning approach according to the syntax of the package tool in use (e.g., Poetry: fastmcp = '2.14.1' or equivalent exact-specifier).
Applied to files:
Server/pyproject.toml
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
Server/pyproject.tomlServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/__init__.py
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
Server/pyproject.toml
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
Server/pyproject.toml
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/pyproject.tomlServer/src/main.pyServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/__init__.py
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/pyproject.tomlServer/src/cli/CLI_USAGE_GUIDE.md
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
Server/src/cli/CLI_USAGE_GUIDE.md
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
Server/src/cli/CLI_USAGE_GUIDE.md
🧬 Code graph analysis (10)
Server/src/cli/utils/connection.py (2)
Server/src/cli/main.py (1)
cli(70-103)Server/src/cli/utils/config.py (2)
get_config(34-39)CLIConfig(9-27)
Server/src/cli/main.py (16)
Server/src/cli/utils/config.py (3)
CLIConfig(9-27)set_config(42-45)get_config(34-39)Server/src/cli/utils/output.py (4)
format_output(8-23)print_error(176-178)print_success(171-173)print_info(186-188)Server/src/cli/utils/connection.py (5)
run_command(99-116)run_check_connection(139-141)run_list_instances(188-190)UnityConnectionError(13-15)warn_if_remote_host(18-37)Server/src/cli/commands/gameobject.py (1)
gameobject(14-16)Server/src/cli/commands/component.py (1)
component(14-16)Server/src/cli/commands/scene.py (1)
scene(13-15)Server/src/cli/commands/asset.py (1)
asset(14-16)Server/src/cli/commands/script.py (1)
script(14-16)Server/src/cli/commands/code.py (1)
code(13-15)Server/src/cli/commands/editor.py (1)
editor(13-15)Server/src/cli/commands/prefab.py (1)
prefab(13-15)Server/src/cli/commands/material.py (1)
material(14-16)Server/src/cli/commands/lighting.py (1)
lighting(13-15)Server/src/cli/commands/animation.py (1)
animation(13-15)Server/src/cli/commands/audio.py (1)
audio(13-15)Server/src/cli/commands/ui.py (1)
ui(13-15)
Server/src/cli/commands/code.py (4)
Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_info(186-188)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)Server/src/cli/commands/script.py (1)
read(91-125)
Server/src/cli/commands/ui.py (5)
Server/src/cli/main.py (1)
cli(70-103)Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)Server/src/cli/commands/component.py (1)
component(14-16)
Server/src/cli/commands/gameobject.py (3)
Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
Server/src/cli/commands/material.py (3)
Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
Server/src/cli/commands/script.py (3)
Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (2)
format_output(8-23)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
Server/src/cli/commands/asset.py (3)
Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
Server/src/cli/utils/__init__.py (3)
Server/src/cli/utils/config.py (3)
CLIConfig(9-27)get_config(34-39)set_config(42-45)Server/src/cli/utils/connection.py (4)
run_command(99-116)run_check_connection(139-141)run_list_instances(188-190)UnityConnectionError(13-15)Server/src/cli/utils/output.py (5)
format_output(8-23)print_success(171-173)print_error(176-178)print_warning(181-183)print_info(186-188)
Server/src/cli/commands/prefab.py (3)
Server/src/cli/utils/config.py (1)
get_config(34-39)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
🪛 markdownlint-cli2 (0.18.1)
Server/src/cli/CLI_USAGE_GUIDE.md
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
Server/src/cli/commands/animation.py
33-33: Unused function argument: layer
(ARG001)
83-83: Local variable config is assigned to but never used
Remove assignment to unused variable config
(F841)
Server/src/main.py
336-336: Do not catch blind exception: Exception
(BLE001)
337-337: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
356-356: Do not catch blind exception: Exception
(BLE001)
Server/src/cli/utils/connection.py
29-29: Possible binding to all interfaces
(S104)
81-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
81-85: Avoid specifying long messages outside the exception class
(TRY003)
87-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-90: Avoid specifying long messages outside the exception class
(TRY003)
92-94: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
92-94: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Do not catch blind exception: Exception
(BLE001)
96-96: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
96-96: Avoid specifying long messages outside the exception class
(TRY003)
135-135: Do not catch blind exception: Exception
(BLE001)
182-183: try-except-continue detected, consider logging the exception
(S112)
182-182: Do not catch blind exception: Exception
(BLE001)
185-185: Avoid specifying long messages outside the exception class
(TRY003)
Server/src/cli/commands/ui.py
91-91: Unused function argument: position
(ARG001)
147-147: Unused function argument: text
(ARG001)
195-195: Unused function argument: sprite
(ARG001)
Server/src/cli/commands/audio.py
31-31: Unused function argument: clip
(ARG001)
Server/tests/test_cli.py
738-738: Unused method argument: mock_unity_response
(ARG002)
745-745: Unused method argument: mock_unity_response
(ARG002)
764-764: Unused method argument: mock_unity_response
(ARG002)
Server/src/cli/utils/__init__.py
18-31: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
Server/src/cli/utils/output.py
188-188: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (34)
Server/pyproject.toml (2)
39-39: LGTM! Click dependency added appropriately.The Click dependency is added with a reasonable version constraint. Unlike
fastmcp(which requires exact pinning per project learnings), Click has strong semantic versioning guarantees and backwards compatibility, making the>=8.1.0constraint appropriate for a CLI framework.Based on learnings, only fastmcp requires exact version pinning to prevent breaking changes affecting MCP tools.
55-55: LGTM! Console script entry point configured correctly.The new
unity-mcpentry point is properly configured to invoke the CLI main function.Server/src/cli/CLI_USAGE_GUIDE.md (1)
1-734: LGTM! Comprehensive and well-structured CLI documentation.The usage guide is thorough, covering installation, syntax, common pitfalls, and extensive command examples. The documentation clearly explains argument vs. option syntax and multi-value parameter usage, which helps prevent common CLI mistakes.
Note: Static analysis warnings about missing language specifiers on lines 83 and 129 are false positives—those blocks show expected output and abstract patterns, not executable code.
Server/src/cli/utils/connection.py (6)
13-15: LGTM! Custom exception defined appropriately.
18-37: LGTM! Security warning appropriately implemented.The function correctly warns users when connecting to non-localhost servers. The static analysis warning about "0.0.0.0" (S104) is a false positive—the code checks whether the configured host equals "0.0.0.0", it doesn't bind to that address.
99-116: LGTM! Synchronous wrapper implemented correctly.
119-136: LGTM! Connection check implemented appropriately.The broad exception catch on line 135 is acceptable for a health check function—returning
Falseon any error is the correct behavior for a simple connectivity test.
139-141: LGTM! Synchronous wrapper implemented correctly.
188-190: LGTM! Synchronous wrapper implemented correctly.Server/src/cli/__init__.py (1)
1-3: LGTM! Package initializer properly configured.The CLI package is initialized with appropriate docstring and version metadata.
Server/src/cli/commands/__init__.py (1)
1-3: LGTM! Commands package initializer properly configured.The commands package is correctly initialized with a clear note that command registration happens in main.py, consistent with the pluggable architecture described in the PR.
Server/src/cli/commands/code.py (1)
63-64: code.py is correct; the bug is in script.py instead.Line 63 correctly checks for
"contents"(plural), which matches the actual response structure from themanage_scriptbackend (seeServer/src/services/tools/script_apply_edits.py:630). However, the relatedreadcommand inServer/src/cli/commands/script.py:117incorrectly checks for"content"(singular), which will cause it to fail silently and fall back to formatted output instead of displaying the script content directly. The script.py command should use"contents"to match the backend response and the code.py implementation.Likely an incorrect or invalid review comment.
Server/tests/test_cli.py (2)
1-27: Well-structured test fixtures and imports.The test suite is well-organized with clear section markers, appropriate fixtures for mocking CLI behavior, and comprehensive coverage of the CLI surface area.
806-874: Good integration-style test coverage with realistic workflows.The integration tests properly simulate multi-step workflows (create → modify → delete) with realistic response payloads, verifying both successful exit codes and expected output content.
Server/src/cli/commands/asset.py (1)
1-16: Clean module structure with consistent patterns.The asset command group follows a consistent pattern with proper error handling, JSON validation for properties, and appropriate confirmation prompts for destructive operations.
Server/src/cli/commands/prefab.py (1)
1-16: Well-implemented prefab command group.The prefab commands follow the established CLI patterns with consistent error handling and appropriate options for each operation.
Server/src/cli/utils/__init__.py (1)
1-31: Clean re-export module consolidating CLI utilities.The
__init__.pyproperly centralizes imports for easier consumption by command modules. The__all__list comprehensively covers the public API.Server/src/cli/commands/scene.py (2)
1-16: Comprehensive scene command group with good option coverage.The scene commands provide useful functionality with sensible defaults and consistent error handling patterns.
99-138: Smart scene loading with appropriate path/name detection.The load command intelligently differentiates between paths (ending in
.unity) and scene names, with proper validation for build index parsing.Server/src/cli/commands/script.py (3)
1-16: LGTM - Clean module structure and imports.The module setup follows the established patterns from other CLI command modules with appropriate imports and a well-defined Click group.
179-183: Good JSON validation with clear error messaging.The edit command properly validates JSON input before attempting the command, providing a clear error message on parse failure.
218-222: Unusedlevelparameter in validate command.The
leveloption is defined and parsed but not included in theparamsdictionary sent to the backend.🐛 Proposed fix
params: dict[str, Any] = { "uri": path, + "level": level, "include_diagnostics": True, }Likely an incorrect or invalid review comment.
Server/src/cli/main.py (2)
106-134: Status command implementation is well-structured.The status command provides useful feedback about server connectivity and displays connected Unity instances with relevant details (project, version, truncated hash). Good use of
print_infofor non-connected state.
163-170: Import inside function - acceptable for rarely-used command.The
jsonimport insideraw_commandis unconventional but acceptable since this is likely a power-user command that won't be called frequently.Server/src/cli/commands/component.py (3)
1-16: LGTM - Standard module structure.Clean imports and group definition following established patterns.
137-142: Good fallback strategy for value parsing.The silent fallback to string when JSON parsing fails is appropriate here, as users may pass simple string values that don't need JSON encoding.
194-200: No action needed—backend handler supports both property signatures.The
manage_componentsbackend handler already supports both the single property format (propertyandvaluefields) and batch properties format (propertiesdict). Integration tests confirm both signatures work correctly:
test_manage_components_set_property_singlevalidates the single property pathtest_manage_components_set_property_multiplevalidates the batch properties pathThe implementation explicitly handles both cases in sequence, so there is no API incompatibility.
Server/src/cli/commands/material.py (1)
141-149: Robust 3-tier value parsing.Good approach: try JSON first for complex types, then float for numeric values, then fall back to string. This provides flexibility for different property types.
Server/src/cli/commands/gameobject.py (1)
430-497: Well-designed relative move command.The move command has good directional options and clear separation of world/local space. The required
--referenceand--directionoptions ensure the command has the necessary context.Server/src/cli/utils/output.py (3)
26-31: Good defensive JSON serialization.The fallback to error object on serialization failure is a good defensive pattern that prevents crashes while providing debugging information.
60-70: Large list truncation is well-implemented.Shows first 5 and last 5 items with a clear indication of how many items are omitted. This balances information density with readability.
186-188: Intentional use of Unicode information symbol.The static analysis warning about the ambiguous
ℹ(INFORMATION SOURCE) character is a false positive - this is the intended emoji for the info message indicator. The character renders correctly in modern terminals.Server/src/cli/commands/editor.py (2)
63-132: Well-designed console command with dual functionality.The console command cleanly handles both reading and clearing the console using the
--clearflag, with early return after clear operation. The multiple filtering options (type, count, filter text, stacktrace) provide good flexibility.
266-299: Tests command implementation looks good.The test runner command appropriately handles optional timeout and mode selection. The results are formatted according to the global format setting.
| @click.option( | ||
| "--position", | ||
| nargs=2, | ||
| type=float, | ||
| default=(0, 0), | ||
| help="Anchored position X Y." | ||
| ) | ||
| def create_text(name: str, parent: str, text: str, position: tuple): | ||
| """Create a UI Text element (TextMeshPro). | ||
| \b | ||
| Examples: | ||
| unity-mcp ui create-text "TitleText" --parent "MainUI" --text "Hello World" | ||
| """ | ||
| config = get_config() | ||
|
|
||
| try: | ||
| # Step 1: Create empty GameObject with parent | ||
| result = run_command("manage_gameobject", { | ||
| "action": "create", | ||
| "name": name, | ||
| "parent": parent, | ||
| }, config) | ||
|
|
||
| if not (result.get("success") or result.get("data") or result.get("result")): | ||
| click.echo(format_output(result, config.format)) | ||
| return | ||
|
|
||
| # Step 2: Add RectTransform and TextMeshProUGUI | ||
| run_command("manage_components", { | ||
| "action": "add", | ||
| "target": name, | ||
| "componentType": "TextMeshProUGUI", | ||
| }, config) | ||
|
|
||
| # Step 3: Set text content | ||
| run_command("manage_components", { | ||
| "action": "set_property", | ||
| "target": name, | ||
| "componentType": "TextMeshProUGUI", | ||
| "property": "text", | ||
| "value": text, | ||
| }, config) | ||
|
|
||
| click.echo(format_output(result, config.format)) | ||
| print_success(f"Created Text: {name}") | ||
| except UnityConnectionError as e: | ||
| print_error(str(e)) | ||
| sys.exit(1) |
There was a problem hiding this comment.
The --position and --text options are declared but never used in create_text.
The position tuple is accepted but not applied to the RectTransform, and while text is correctly used in line 125, the position functionality is missing. The position should be applied after component creation.
🔧 Proposed fix to apply position
# Step 3: Set text content
run_command("manage_components", {
"action": "set_property",
"target": name,
"componentType": "TextMeshProUGUI",
"property": "text",
"value": text,
}, config)
+
+ # Step 4: Set position if specified
+ if position != (0, 0):
+ run_command("manage_components", {
+ "action": "set_property",
+ "target": name,
+ "componentType": "RectTransform",
+ "property": "anchoredPosition",
+ "value": {"x": position[0], "y": position[1]},
+ }, config)🧰 Tools
🪛 Ruff (0.14.10)
91-91: Unused function argument: position
(ARG001)
🤖 Prompt for AI Agents
In @Server/src/cli/commands/ui.py around lines 84 - 132, create_text accepts a
position tuple but never applies it to the RectTransform; after adding the
TextMeshProUGUI component (in create_text) call run_command("manage_components",
{ "action": "set_property", "target": name, "componentType": "RectTransform",
"property": "anchoredPosition", "value": list(position) }, config) (or use
"anchoredPosition3D"/"localPosition" if your backend expects 3D) and ensure you
pass the position variable (converted to the expected list/format) so the
RectTransform receives the anchored position.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @Server/src/cli/commands/lighting.py:
- Around line 94-106: The handler currently returns unconditionally after the
color set block, causing premature exit and skipping subsequent intensity
handling and final output; update the color block in the function (the code
around run_command("manage_components", ...) that sets color and the
color_result check) so the return is only executed when color_result indicates
failure (i.e., place the return inside the if not color_result.get("success")
block), leaving successful flows to continue to the intensity step and final
output.
- Around line 108-120: The intensity handling always exits the function because
the stray return after calling run_command("manage_components", ...) is outside
the error-check, so move the return inside the if not
intensity_result.get("success") block: call
click.echo(format_output(intensity_result, config.format)) and then return only
on error; otherwise allow the function to continue to the final output/success
path. Ensure you update the block around intensity_result,
run_command("manage_components", ...), format_output and click.echo so normal
success falls through.
In @Server/src/cli/commands/ui.py:
- Around line 196-229: The create_image function accepts a sprite parameter but
never applies it; after successfully adding the Image component (run_command
call with "manage_components", "componentType":"Image"), if sprite is provided
call the appropriate run_command to load/resolve the asset and then set the
Image component's sprite property on the created GameObject (e.g., a follow-up
run_command to "manage_components" with an action like "set" or "setProperty"
targeting the created object/name, component "Image", property "sprite", and
value sprite). Check the result of these calls, log/echo failures with
format_output and print_error, and exit with sys.exit(1) on UnityConnectionError
or unsuccessful asset/property-set responses to maintain existing error
handling.
- Around line 148-181: create_button accepts a text parameter but never applies
it to the created button; update create_button to set the button label after
adding components by creating a child GameObject (e.g., "Text") under the button
or by adding/updating the text component (e.g., "Text" or "TextMeshProUGUI") and
setting its "text" property via run_command calls: after the manage_components
loop in create_button, create the child text object (use manage_gameobject with
parent=name), add the appropriate text component with manage_components, then
call run_command to set the component property "text" to the passed text value
(use existing config and error handling).
🧹 Nitpick comments (2)
Server/src/cli/utils/config.py (1)
18-38: Consider chaining exceptions for better debugging.Per the static analysis hint, using
raise ... from errpreserves the original exception context, which helps when diagnosing configuration issues.♻️ Proposed fix
port_raw = os.environ.get("UNITY_MCP_HTTP_PORT", "8080") try: port = int(port_raw) - except (ValueError, TypeError): - raise ValueError(f"Invalid UNITY_MCP_HTTP_PORT value: {port_raw!r}") + except (ValueError, TypeError) as err: + raise ValueError(f"Invalid UNITY_MCP_HTTP_PORT value: {port_raw!r}") from err timeout_raw = os.environ.get("UNITY_MCP_TIMEOUT", "30") try: timeout = int(timeout_raw) - except (ValueError, TypeError): - raise ValueError(f"Invalid UNITY_MCP_TIMEOUT value: {timeout_raw!r}") + except (ValueError, TypeError) as err: + raise ValueError(f"Invalid UNITY_MCP_TIMEOUT value: {timeout_raw!r}") from errServer/src/cli/commands/animation.py (1)
75-86: Unusedconfigvariable in placeholder command.Since this is a placeholder, the unused
configassignment is noise. Either remove it or implement the actual command logic.♻️ Option 1: Remove unused config
def set_parameter(target: str, param_name: str, value: str, param_type: str): """Set an Animator parameter. \b Examples: unity-mcp animation set-parameter "Player" "Speed" 5.0 unity-mcp animation set-parameter "Player" "IsRunning" true --type bool unity-mcp animation set-parameter "Player" "Jump" "" --type trigger """ - config = get_config() print_info("Animation parameter command - requires custom Unity implementation") click.echo(f"Would set {param_name}={value} ({param_type}) on {target}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Server/src/cli/commands/animation.pyServer/src/cli/commands/gameobject.pyServer/src/cli/commands/lighting.pyServer/src/cli/commands/material.pyServer/src/cli/commands/script.pyServer/src/cli/commands/ui.pyServer/src/cli/utils/config.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Server/src/cli/commands/material.py
- Server/src/cli/commands/script.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
🧬 Code graph analysis (3)
Server/src/cli/commands/lighting.py (3)
Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
Server/src/cli/commands/animation.py (6)
Server/src/cli/main.py (1)
cli(70-103)Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_info(186-188)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)Server/src/cli/commands/audio.py (1)
play(31-57)Server/src/cli/commands/editor.py (1)
play(19-30)
Server/src/cli/commands/gameobject.py (3)
Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (4)
format_output(8-23)print_error(176-178)print_success(171-173)print_warning(181-183)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
🪛 Ruff (0.14.10)
Server/src/cli/commands/ui.py
148-148: Unused function argument: text
(ARG001)
196-196: Unused function argument: sprite
(ARG001)
Server/src/cli/commands/animation.py
84-84: Local variable config is assigned to but never used
Remove assignment to unused variable config
(F841)
Server/src/cli/utils/config.py
24-24: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
24-24: Avoid specifying long messages outside the exception class
(TRY003)
30-30: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
30-30: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (14)
Server/src/cli/commands/lighting.py (2)
12-15: LGTM!The lighting group definition follows the standard Click pattern used across other command modules.
18-55: LGTM!Good command definition with appropriate options for light type, position, color, and intensity. Examples in the docstring are helpful.
Server/src/cli/utils/config.py (2)
8-16: LGTM!Clean dataclass design with sensible defaults. The
formatfield supporting text/json/table aligns well with the output utilities.
45-56: LGTM!The lazy initialization pattern with
get_config()and explicitset_config()works well for CLI usage where the main entry point sets the config once.Server/src/cli/commands/animation.py (2)
12-15: LGTM!Animation group definition follows the established pattern.
34-61: LGTM!The play command correctly constructs the invoke_method payload for the Animator component and handles errors appropriately.
Server/src/cli/commands/ui.py (3)
12-15: LGTM!UI group definition follows the established pattern.
26-69: LGTM!The create_canvas command properly creates a GameObject and adds the required Canvas, CanvasScaler, and GraphicRaycaster components with render mode configuration.
91-133: LGTM!The create_text command correctly creates a text element with parent, adds TextMeshProUGUI, and sets the text content.
Server/src/cli/commands/gameobject.py (5)
13-16: LGTM!The gameobject group definition follows the established pattern.
44-68: LGTM!The find command has a comprehensive set of search options and properly handles pagination with cursor/limit.
129-202: LGTM!Good implementation with defensive handling of component addition failures. The warning message for failed components provides useful feedback without blocking the overall creation.
346-375: LGTM!Good UX pattern with the confirmation prompt and
--forceflag for destructive operations.
436-503: LGTM!The move command with relative positioning is well-designed. The direction choices cover all expected movement directions and the local/world space toggle is useful.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Server/src/main.py:
- Around line 299-338: In cli_command_route, explicitly handle malformed JSON by
wrapping await request.json() in a try/except that catches json.JSONDecodeError
(or ValueError depending on the request.json implementation) and returns a
JSONResponse with success: False, an explanatory error and status_code=400; then
replace the generic logger.error call in the broad exception handler with
logger.exception(...) so the stack trace is preserved when an unexpected
exception occurs. Ensure you import json if using json.JSONDecodeError and keep
the existing behavior for finding sessions and sending commands in
send_command/PluginHub.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/src/main.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/main.py
🪛 Ruff (0.14.10)
Server/src/main.py
336-336: Do not catch blind exception: Exception
(BLE001)
337-337: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
356-356: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
Server/src/main.py (2)
341-357: LGTM!The endpoint is straightforward. The broad exception catch is acceptable for this simple listing endpoint where any unexpected failure should return a 500. For consistency with the previous endpoint, you could optionally use
logger.exception()if you want tracebacks in logs.
493-502: Good defensive parsing.Graceful handling of invalid
UNITY_MCP_HTTP_PORTvalues with a warning log and fallback to defaults is a solid improvement. This prevents startup crashes from misconfigured environment variables.Also applies to: 537-537
| @mcp.custom_route("/api/command", methods=["POST"]) | ||
| async def cli_command_route(request: Request) -> JSONResponse: | ||
| """REST endpoint for CLI commands to Unity.""" | ||
| try: | ||
| body = await request.json() | ||
| command_type = body.get("type") | ||
| params = body.get("params", {}) | ||
| unity_instance = body.get("unity_instance") | ||
|
|
||
| if not command_type: | ||
| return JSONResponse({"success": False, "error": "Missing 'type' field"}, status_code=400) | ||
|
|
||
| # Get available sessions | ||
| sessions = await PluginHub.get_sessions() | ||
| if not sessions.sessions: | ||
| return JSONResponse({ | ||
| "success": False, | ||
| "error": "No Unity instances connected. Make sure Unity is running with MCP plugin." | ||
| }, status_code=503) | ||
|
|
||
| # Find target session | ||
| session_id = None | ||
| if unity_instance: | ||
| # Try to match by hash or project name | ||
| for sid, details in sessions.sessions.items(): | ||
| if details.hash == unity_instance or details.project == unity_instance: | ||
| session_id = sid | ||
| break | ||
|
|
||
| if not session_id: | ||
| # Use first available session | ||
| session_id = next(iter(sessions.sessions.keys())) | ||
|
|
||
| # Send command to Unity | ||
| result = await PluginHub.send_command(session_id, command_type, params) | ||
| return JSONResponse(result) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"CLI command error: {e}") | ||
| return JSONResponse({"success": False, "error": str(e)}, status_code=500) |
There was a problem hiding this comment.
Improve error handling for JSON parsing and exception logging.
Two issues here:
-
await request.json()can raisejson.JSONDecodeErrorfor malformed payloads, which currently falls through to the generic exception handler returning an unhelpful message. -
Per static analysis, use
logger.exception()instead oflogger.error()to preserve the stack trace in logs.
Proposed fix
+import json
+
@mcp.custom_route("/api/command", methods=["POST"])
async def cli_command_route(request: Request) -> JSONResponse:
"""REST endpoint for CLI commands to Unity."""
try:
body = await request.json()
+ except json.JSONDecodeError as e:
+ return JSONResponse({"success": False, "error": f"Invalid JSON: {e}"}, status_code=400)
+
+ try:
command_type = body.get("type")
params = body.get("params", {})
# ... rest of logic ...
except Exception as e:
- logger.error(f"CLI command error: {e}")
+ logger.exception("CLI command error")
return JSONResponse({"success": False, "error": str(e)}, status_code=500)🧰 Tools
🪛 Ruff (0.14.10)
336-336: Do not catch blind exception: Exception
(BLE001)
337-337: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In @Server/src/main.py around lines 299 - 338, In cli_command_route, explicitly
handle malformed JSON by wrapping await request.json() in a try/except that
catches json.JSONDecodeError (or ValueError depending on the request.json
implementation) and returns a JSONResponse with success: False, an explanatory
error and status_code=400; then replace the generic logger.error call in the
broad exception handler with logger.exception(...) so the stack trace is
preserved when an unexpected exception occurs. Ensure you import json if using
json.JSONDecodeError and keep the existing behavior for finding sessions and
sending commands in send_command/PluginHub.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @Server/src/cli/commands/code.py:
- Around line 43-45: The code currently removes only the ".cs" suffix when
deriving the resource name (variables parts/filename/directory), which breaks
support for other file types; change the logic that builds filename to strip any
file extension (e.g., use a proper splitext-style operation on parts[-1] so
"Player.shader" -> "Player") and keep directory computed from
"/".join(parts[:-1]) or "Assets" unchanged; update references to filename so all
file types produce the base name without their extension.
In @Server/src/cli/commands/ui.py:
- Around line 219-233: The manage_components "add" and subsequent "set_property"
calls invoked via run_command should check the returned result and handle
failures consistently; update the block around run_command("manage_components",
{ "action": "add", ... }, config) to capture the return value, verify success
(e.g., status/ok flag or error field), log or raise an error and abort further
steps if adding the Image component failed, and likewise capture and validate
the result of run_command("manage_components", { "action": "set_property", ...
}, config) before proceeding; reference the existing run_command invocations for
"add" and "set_property" in this section and mirror the error-handling used
elsewhere in this module (raise/print/processLogger usage) so the sprite step
only runs when the add succeeded and set_property failures are surfaced.
- Around line 114-127: The add-and-set sequence using
run_command("manage_components", {..., "action":"add",
"componentType":"TextMeshProUGUI", "target": name}, config) and the subsequent
run_command(... "action":"set_property", "property":"text", "value": text, ...)
can fail silently; update the code around those calls to capture and validate
each run_command response (for both the add and set_property calls), check
success/failure and that the returned component reference matches expectations,
and if a failure or mismatch occurs raise/log an error and abort further steps;
mirror the same result-checking pattern you applied in create_canvas so failures
are surfaced and handled.
- Around line 48-63: The loop that calls run_command("manage_components", ...)
for each component (Canvas, CanvasScaler, GraphicRaycaster) does not check the
return/result and can hide partial failures; update the component-adding loop in
the function that constructs the UI (where run_command and render_mode_value are
used) to inspect each run_command result, and on any failure either return/raise
immediately (failing fast) or collect errors and abort before proceeding to the
renderMode step—ensure you reference the run_command("manage_components", {...})
calls and the subsequent render_mode_value block so the code only sets
Canvas.renderMode after all component additions succeeded.
🧹 Nitpick comments (8)
Server/src/cli/commands/lighting.py (3)
96-116: Fix inconsistent indentation in nested dictionaries.The dictionary contents in the
run_commandcalls are at the same indentation level as the opening brace, which hurts readability and is inconsistent with surrounding code.🔧 Suggested fix
# Step 4: Set color if provided if color: color_result = run_command("manage_components", { - "action": "set_property", - "target": name, - "componentType": "Light", - "property": "color", - "value": {"r": color[0], "g": color[1], "b": color[2], "a": 1}, + "action": "set_property", + "target": name, + "componentType": "Light", + "property": "color", + "value": {"r": color[0], "g": color[1], "b": color[2], "a": 1}, }, config) if not color_result.get("success"): click.echo(format_output(color_result, config.format)) return # Step 5: Set intensity if provided if intensity is not None: intensity_result = run_command("manage_components", { - "action": "set_property", - "target": name, - "componentType": "Light", - "property": "intensity", - "value": intensity, + "action": "set_property", + "target": name, + "componentType": "Light", + "property": "intensity", + "value": intensity, }, config)
58-124: Consider cleanup on partial failure.This multi-step operation can leave the scene in an inconsistent state if an intermediate step fails—step 1 creates the GameObject, but if steps 2–5 fail, the user is left with an orphaned empty object. Other command modules use single
run_commandcalls, avoiding this issue.Consider either:
- Adding a cleanup attempt (delete the GameObject) on failure, or
- Documenting this behavior in the docstring so users know to clean up manually.
This is a minor UX improvement and can be deferred.
66-66: Remove redundant parentheses.The parentheses around
create_result.get("success")are unnecessary.🔧 Suggested fix
- if not (create_result.get("success")): + if not create_result.get("success"):Server/src/cli/commands/code.py (2)
8-8: Unused import:print_info
print_infois imported but never used in this module. Consider removing it to keep imports clean.Suggested fix
-from cli.utils.output import format_output, print_error, print_info +from cli.utils.output import format_output, print_error
53-56: Falsy check may exclude explicit zero values.Using
if start_line:andif line_count:excludes0. While0is likely invalid for a 1-based line number, an explicitis not Nonecheck is more precise and self-documenting.Suggested fix
- if start_line: + if start_line is not None: params["startLine"] = start_line - if line_count: + if line_count is not None: params["lineCount"] = line_countServer/src/cli/commands/audio.py (2)
1-9: Stale docstring and unused import.The docstring states "placeholder for future implementation" but the module contains a full implementation. Also,
print_infois imported but never used.Suggested fix
-"""Audio CLI commands - placeholder for future implementation.""" +"""Audio CLI commands for AudioSource control and audio settings.""" import sys import click from typing import Optional, Any from cli.utils.config import get_config -from cli.utils.output import format_output, print_error, print_info +from cli.utils.output import format_output, print_error from cli.utils.connection import run_command, UnityConnectionError
99-108: Consider validating volume level range.Unity's
AudioSource.volumeaccepts values between 0.0 and 1.0. Useclick.FloatRangeto enforce this constraint and provide immediate user feedback for invalid values.Suggested change
@audio.command("volume") @click.argument("target") -@click.argument("level", type=float) +@click.argument("level", type=click.FloatRange(0.0, 1.0)) @click.option( "--search-method", type=click.Choice(["by_name", "by_path", "by_id"]), default=None, help="How to find the target." )Server/src/cli/commands/ui.py (1)
1-5: Minor cleanup: stale docstring and unused import.The module docstring says "placeholder for future implementation" but the commands are implemented. Also,
Anyis imported but never used.Suggested fix
-"""UI CLI commands - placeholder for future implementation.""" +"""UI CLI commands - create and modify Unity UI elements.""" import sys import click -from typing import Optional, Any +from typing import Optional
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Server/src/cli/commands/audio.pyServer/src/cli/commands/code.pyServer/src/cli/commands/lighting.pyServer/src/cli/commands/ui.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
🧬 Code graph analysis (2)
Server/src/cli/commands/audio.py (3)
Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_info(186-188)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)
Server/src/cli/commands/lighting.py (10)
Server/src/cli/main.py (1)
cli(70-103)Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_success(171-173)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)Server/src/cli/commands/material.py (1)
create(53-84)Server/src/cli/commands/script.py (1)
create(43-74)Server/src/cli/commands/gameobject.py (1)
create(129-202)Server/src/cli/commands/asset.py (1)
create(114-145)Server/src/cli/commands/prefab.py (1)
create(115-143)Server/src/cli/commands/scene.py (1)
create(178-202)
🪛 Ruff (0.14.10)
Server/src/cli/commands/ui.py
148-148: Unused function argument: text
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
Server/src/cli/commands/code.py (3)
12-15: LGTM!Standard Click group definition following conventions.
20-31: Consider validating positive integers for line options.
--start-lineand--line-countaccept any integer, including zero or negative values, which may cause unexpected behavior. If the server doesn't validate these, consider adding client-side validation.Optional: Add min-value validation
@click.option( "--start-line", "-s", default=None, type=int, + callback=lambda ctx, param, value: value if value is None or value >= 1 else ctx.fail("start-line must be >= 1"), help="Starting line number (1-based)." ) @click.option( "--line-count", "-n", default=None, type=int, + callback=lambda ctx, param, value: value if value is None or value >= 1 else ctx.fail("line-count must be >= 1"), help="Number of lines to read." )Alternatively, verify server-side handling of invalid values.
58-71: Error handling and output logic looks good.The try/except properly catches
UnityConnectionErrorand exits with a non-zero code. The output logic correctly prioritizes raw content for readability when available, falling back to formatted output otherwise.Server/src/cli/commands/audio.py (3)
12-15: LGTM!The Click group definition is clean and the docstring appropriately describes the command group's purpose.
18-60: LGTM!The play command implementation follows good patterns with proper error handling and helpful examples. The command structure integrates well with the CLI utilities.
63-96: LGTM!The stop command follows the same pattern as play with consistent error handling.
Server/src/cli/commands/ui.py (1)
12-15: LGTM!Standard Click group definition with appropriate docstring.
| for component in ["Canvas", "CanvasScaler", "GraphicRaycaster"]: | ||
| run_command("manage_components", { | ||
| "action": "add", | ||
| "target": name, | ||
| "componentType": component, | ||
| }, config) | ||
|
|
||
| # Step 3: Set render mode | ||
| render_mode_value = {"ScreenSpaceOverlay": 0, "ScreenSpaceCamera": 1, "WorldSpace": 2}.get(render_mode, 0) | ||
| run_command("manage_components", { | ||
| "action": "set_property", | ||
| "target": name, | ||
| "componentType": "Canvas", | ||
| "property": "renderMode", | ||
| "value": render_mode_value, | ||
| }, config) |
There was a problem hiding this comment.
Intermediate run_command calls are not checked for errors.
If adding a component (e.g., CanvasScaler) fails, the loop continues and the user sees a success message despite partial failure. This can leave Unity in an inconsistent state.
Consider checking each result and failing early or accumulating errors:
Suggested approach
# Step 2: Add Canvas components
for component in ["Canvas", "CanvasScaler", "GraphicRaycaster"]:
- run_command("manage_components", {
+ comp_result = run_command("manage_components", {
"action": "add",
"target": name,
"componentType": component,
}, config)
+ if not comp_result.get("success"):
+ print_error(f"Failed to add {component}: {comp_result}")
+ sys.exit(1)
# Step 3: Set render mode
render_mode_value = {"ScreenSpaceOverlay": 0, "ScreenSpaceCamera": 1, "WorldSpace": 2}.get(render_mode, 0)
- run_command("manage_components", {
+ mode_result = run_command("manage_components", {
"action": "set_property",
"target": name,
"componentType": "Canvas",
"property": "renderMode",
"value": render_mode_value,
}, config)
+ if not mode_result.get("success"):
+ print_error(f"Failed to set render mode: {mode_result}")
+ sys.exit(1)🤖 Prompt for AI Agents
In @Server/src/cli/commands/ui.py around lines 48 - 63, The loop that calls
run_command("manage_components", ...) for each component (Canvas, CanvasScaler,
GraphicRaycaster) does not check the return/result and can hide partial
failures; update the component-adding loop in the function that constructs the
UI (where run_command and render_mode_value are used) to inspect each
run_command result, and on any failure either return/raise immediately (failing
fast) or collect errors and abort before proceeding to the renderMode
step—ensure you reference the run_command("manage_components", {...}) calls and
the subsequent render_mode_value block so the code only sets Canvas.renderMode
after all component additions succeeded.
| run_command("manage_components", { | ||
| "action": "add", | ||
| "target": name, | ||
| "componentType": "TextMeshProUGUI", | ||
| }, config) | ||
|
|
||
| # Step 3: Set text content | ||
| run_command("manage_components", { | ||
| "action": "set_property", | ||
| "target": name, | ||
| "componentType": "TextMeshProUGUI", | ||
| "property": "text", | ||
| "value": text, | ||
| }, config) |
There was a problem hiding this comment.
Same error handling gap: intermediate steps not verified.
The TextMeshProUGUI component addition (line 114-118) and text property setting (line 121-127) could silently fail. Consider checking results for consistency with any fix applied to create_canvas.
🤖 Prompt for AI Agents
In @Server/src/cli/commands/ui.py around lines 114 - 127, The add-and-set
sequence using run_command("manage_components", {..., "action":"add",
"componentType":"TextMeshProUGUI", "target": name}, config) and the subsequent
run_command(... "action":"set_property", "property":"text", "value": text, ...)
can fail silently; update the code around those calls to capture and validate
each run_command response (for both the add and set_property calls), check
success/failure and that the returned component reference matches expectations,
and if a failure or mismatch occurs raise/log an error and abort further steps;
mirror the same result-checking pattern you applied in create_canvas so failures
are surfaced and handled.
| run_command("manage_components", { | ||
| "action": "add", | ||
| "target": name, | ||
| "componentType": "Image", | ||
| }, config) | ||
|
|
||
| # Step 3: Set sprite if provided | ||
| if sprite: | ||
| run_command("manage_components", { | ||
| "action": "set_property", | ||
| "target": name, | ||
| "componentType": "Image", | ||
| "property": "sprite", | ||
| "value": sprite, | ||
| }, config) |
There was a problem hiding this comment.
Same pattern: check results of component/property operations.
Consistent with other commands, the Image component addition and sprite setting should verify success. The conditional sprite handling (lines 226-233) is good.
🤖 Prompt for AI Agents
In @Server/src/cli/commands/ui.py around lines 219 - 233, The manage_components
"add" and subsequent "set_property" calls invoked via run_command should check
the returned result and handle failures consistently; update the block around
run_command("manage_components", { "action": "add", ... }, config) to capture
the return value, verify success (e.g., status/ok flag or error field), log or
raise an error and abort further steps if adding the Image component failed, and
likewise capture and validate the result of run_command("manage_components", {
"action": "set_property", ... }, config) before proceeding; reference the
existing run_command invocations for "add" and "set_property" in this section
and mirror the error-handling used elsewhere in this module
(raise/print/processLogger usage) so the sprite step only runs when the add
succeeded and set_property failures are surfaced.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @Server/src/cli/commands/animation.py:
- Line 84: Remove the unused local variable by deleting the assignment "config =
get_config()" (the unused symbol is config) in
Server/src/cli/commands/animation.py; if the call to get_config() has side
effects that must be preserved, call get_config() without assigning its return
value (e.g., call get_config() alone) instead of assigning it to config.
In @Server/src/cli/commands/code.py:
- Around line 54-57: The conditionals that set params["startLine"] and
params["lineCount"] use truthiness checks (if start_line / if line_count) which
will skip valid zero values; change both checks to explicit None checks (if
start_line is not None and if line_count is not None) so zero is accepted and
the parameters are set correctly for the existing variables start_line and
line_count.
- Around line 64-65: The response handling in script.py for the manage_script
command checks the wrong key name "content" (singular) so it never reads the
returned file payload; update the check to look for "contents" (plural) and echo
data["contents"] when isinstance(data, dict) and "contents" in data so it
matches code.py and the actual API response format.
In @Server/src/main.py:
- Around line 320-331: When a unity_instance is specified but no matching
session is found the code currently falls back to the first session (session_id
= next(...)), which can send commands to the wrong instance; after the loop that
sets session_id, if unity_instance is truthy and session_id is still None,
return a 404 error response (e.g., raise HTTPException or abort with a clear
message) instead of falling back—update the handler around
session_id/unity_instance/sessions.sessions to perform this check and
short-circuit with a 404 when the requested instance cannot be found.
🧹 Nitpick comments (10)
Server/src/cli/commands/ui.py (5)
1-1: Docstring is outdated.The module docstring says "placeholder for future implementation," but the module contains fully implemented commands. Update to reflect actual functionality.
Suggested fix
-"""UI CLI commands - placeholder for future implementation.""" +"""UI CLI commands - create and modify UI elements."""
5-5: Remove unused import.
Anyis imported but never used in this module.Suggested fix
-from typing import Optional, Any +from typing import Optional
47-53: Intermediate operations lack error checking.Component additions (Canvas, CanvasScaler, GraphicRaycaster) don't verify success. If one fails silently, subsequent operations may produce confusing errors. Consider checking each result or at minimum catching and reporting failures.
This same pattern appears in
create_text,create_button, andcreate_image.Example improvement for one component addition
# Step 2: Add Canvas components for component in ["Canvas", "CanvasScaler", "GraphicRaycaster"]: - run_command("manage_components", { + comp_result = run_command("manage_components", { "action": "add", "target": name, "componentType": component, }, config) + if not (comp_result.get("success") or comp_result.get("data") or comp_result.get("result")): + print_error(f"Failed to add component: {component}") + click.echo(format_output(comp_result, config.format)) + return
148-148: Clarify or remove the inline comment.The comment
#text current placeholderis unclear. If thetextparameter functionality is incomplete, consider adding a TODO with more context; otherwise, remove the comment.Suggested fix
-def create_button(name: str, parent: str, text: str): #text current placeholder +def create_button(name: str, parent: str, text: str):
179-183: Remove unused variable assignment.
label_resultis assigned but never used. Either use it to verify the label creation succeeded or remove the assignment.Suggested fix (if not checking result)
# Step 3: Create child label GameObject label_name = f"{name}_Label" - label_result = run_command("manage_gameobject", { + run_command("manage_gameobject", { "action": "create", "name": label_name, "parent": name, }, config)Server/src/cli/commands/animation.py (2)
1-1: Docstring is inconsistent with actual implementation.The docstring states "placeholder for future implementation," but the
playcommand is fully implemented. Consider updating the docstring to accurately reflect the module's state (e.g., "Animation CLI commands - partially implemented").
64-86: Placeholder command is exposed to users without executing any action.Unlike
playand other CLI commands that callrun_command, this command only prints an info message and echoes a hypothetical action without actually communicating with Unity. Users invoking this command may expect it to work.Consider either:
- Adding a
# TODOcomment and raisingNotImplementedErroror usingsys.exit(1)to indicate the command isn't functional yet.- Implementing the actual command following the pattern in
play.- Not registering this command until it's ready.
Option 1: Make it clearer the command is not implemented
def set_parameter(target: str, param_name: str, value: str, param_type: str): """Set an Animator parameter. \b Examples: unity-mcp animation set-parameter "Player" "Speed" 5.0 unity-mcp animation set-parameter "Player" "IsRunning" true --type bool unity-mcp animation set-parameter "Player" "Jump" "" --type trigger """ - config = get_config() - print_info("Animation parameter command - requires custom Unity implementation") - click.echo(f"Would set {param_name}={value} ({param_type}) on {target}") + # TODO: Implement when custom Unity-side handler is available + print_error("Animation parameter command is not yet implemented - requires custom Unity implementation") + sys.exit(1)Server/src/main.py (2)
337-339: Uselogger.exceptionto preserve traceback.Per static analysis hint,
logger.exceptionautomatically includes the traceback, which aids debugging.♻️ Proposed fix
except Exception as e: - logger.error(f"CLI command error: {e}") + logger.exception("CLI command error: %s", e) return JSONResponse({"success": False, "error": str(e)}, status_code=500)
342-359: LGTM with minor suggestion.The endpoint correctly retrieves and formats session data. Consider using
logger.exceptionin the exception handler for consistency with debugging best practices (similar to the/api/commandendpoint).Server/src/cli/commands/code.py (1)
9-9: Unused import:print_info.
print_infois imported but never used in this module.Suggested fix
-from cli.utils.output import format_output, print_error, print_info +from cli.utils.output import format_output, print_error
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Server/src/cli/commands/animation.pyServer/src/cli/commands/code.pyServer/src/cli/commands/ui.pyServer/src/main.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/main.py
🧬 Code graph analysis (3)
Server/src/cli/commands/code.py (5)
Server/src/cli/main.py (1)
cli(70-103)Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_info(186-188)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)Server/src/cli/commands/script.py (1)
read(91-130)
Server/src/main.py (2)
Server/src/transport/plugin_hub.py (3)
PluginHub(41-558)get_sessions(220-234)send_command(142-217)Server/src/cli/utils/connection.py (1)
send_command(40-96)
Server/src/cli/commands/animation.py (5)
Server/src/cli/utils/config.py (1)
get_config(45-50)Server/src/cli/utils/output.py (3)
format_output(8-23)print_error(176-178)print_info(186-188)Server/src/cli/utils/connection.py (2)
run_command(99-116)UnityConnectionError(13-15)Server/src/cli/commands/audio.py (1)
play(31-60)Server/src/cli/commands/editor.py (1)
play(19-30)
🪛 Ruff (0.14.11)
Server/src/main.py
337-337: Do not catch blind exception: Exception
(BLE001)
338-338: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
357-357: Do not catch blind exception: Exception
(BLE001)
Server/src/cli/commands/ui.py
179-179: Local variable label_result is assigned to but never used
Remove assignment to unused variable label_result
(F841)
Server/src/cli/commands/animation.py
84-84: Local variable config is assigned to but never used
Remove assignment to unused variable config
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
Server/src/cli/commands/ui.py (1)
206-261: LGTM!The
create_imagecommand follows the established pattern correctly, handles the optional sprite parameter appropriately, and properly catches connection errors.Server/src/cli/commands/animation.py (2)
12-15: LGTM!Standard Click group definition follows the pattern used in other command modules.
18-61: LGTM!The
playcommand implementation follows the established pattern from other command modules (e.g.,audio.py). Error handling is consistent with the rest of the CLI.Minor nit: The help text
"Animator layer(TODO)."on line 25 has a missing space and unclear meaning. Consider clarifying (e.g.,"Animator layer index (not yet fully supported).").Server/src/main.py (2)
494-503: Good defensive parsing of environment variable.The safe parsing with warning on invalid values and graceful fallback is well implemented. This prevents crashes from misconfigured environment variables while providing helpful feedback to users.
538-538: Consistent use of sanitized port variable.The use of
_env_porthere aligns with line 503, ensuring both code paths handle the environment variable consistently.Server/src/cli/commands/code.py (2)
13-16: LGTM!Standard Click group definition following the established pattern in other command modules.
59-72: Error handling and output logic are well-structured.The try/except pattern with
UnityConnectionError, the conditional content extraction, and the fallback to formatted output follow the established patterns in the CLI codebase.
| unity-mcp animation set-parameter "Player" "IsRunning" true --type bool | ||
| unity-mcp animation set-parameter "Player" "Jump" "" --type trigger | ||
| """ | ||
| config = get_config() |
There was a problem hiding this comment.
Remove unused variable.
The config variable is assigned but never used. As flagged by static analysis (Ruff F841).
Proposed fix
- config = get_config()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config = get_config() |
🧰 Tools
🪛 Ruff (0.14.11)
84-84: Local variable config is assigned to but never used
Remove assignment to unused variable config
(F841)
🤖 Prompt for AI Agents
In @Server/src/cli/commands/animation.py at line 84, Remove the unused local
variable by deleting the assignment "config = get_config()" (the unused symbol
is config) in Server/src/cli/commands/animation.py; if the call to get_config()
has side effects that must be preserved, call get_config() without assigning its
return value (e.g., call get_config() alone) instead of assigning it to config.
| if start_line: | ||
| params["startLine"] = start_line | ||
| if line_count: | ||
| params["lineCount"] = line_count |
There was a problem hiding this comment.
Use is not None instead of truthiness check for optional integer parameters.
Using if start_line: will skip setting the parameter when start_line=0. While the help states "1-based", using explicit None checks is more defensive and idiomatic.
Suggested fix
- if start_line:
+ if start_line is not None:
params["startLine"] = start_line
- if line_count:
+ if line_count is not None:
params["lineCount"] = line_count📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if start_line: | |
| params["startLine"] = start_line | |
| if line_count: | |
| params["lineCount"] = line_count | |
| if start_line is not None: | |
| params["startLine"] = start_line | |
| if line_count is not None: | |
| params["lineCount"] = line_count |
🤖 Prompt for AI Agents
In @Server/src/cli/commands/code.py around lines 54 - 57, The conditionals that
set params["startLine"] and params["lineCount"] use truthiness checks (if
start_line / if line_count) which will skip valid zero values; change both
checks to explicit None checks (if start_line is not None and if line_count is
not None) so zero is accepted and the parameters are set correctly for the
existing variables start_line and line_count.
| # Find target session | ||
| session_id = None | ||
| if unity_instance: | ||
| # Try to match by hash or project name | ||
| for sid, details in sessions.sessions.items(): | ||
| if details.hash == unity_instance or details.project == unity_instance: | ||
| session_id = sid | ||
| break | ||
|
|
||
| if not session_id: | ||
| # Use first available session | ||
| session_id = next(iter(sessions.sessions.keys())) |
There was a problem hiding this comment.
Silent fallback when specified Unity instance is not found.
When unity_instance is provided but doesn't match any connected session, the code silently falls back to the first available session (lines 329-331). This could lead to commands being sent to the wrong Unity instance unexpectedly. Consider returning a 404 error when the specified instance cannot be found.
🔧 Proposed fix
# Find target session
session_id = None
if unity_instance:
# Try to match by hash or project name
for sid, details in sessions.sessions.items():
if details.hash == unity_instance or details.project == unity_instance:
session_id = sid
break
+ if not session_id:
+ return JSONResponse({
+ "success": False,
+ "error": f"Unity instance '{unity_instance}' not found. Available: {[d.project for d in sessions.sessions.values()]}"
+ }, status_code=404)
- if not session_id:
+ if not session_id and not unity_instance:
# Use first available session
session_id = next(iter(sessions.sessions.keys()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Find target session | |
| session_id = None | |
| if unity_instance: | |
| # Try to match by hash or project name | |
| for sid, details in sessions.sessions.items(): | |
| if details.hash == unity_instance or details.project == unity_instance: | |
| session_id = sid | |
| break | |
| if not session_id: | |
| # Use first available session | |
| session_id = next(iter(sessions.sessions.keys())) | |
| # Find target session | |
| session_id = None | |
| if unity_instance: | |
| # Try to match by hash or project name | |
| for sid, details in sessions.sessions.items(): | |
| if details.hash == unity_instance or details.project == unity_instance: | |
| session_id = sid | |
| break | |
| if not session_id: | |
| return JSONResponse({ | |
| "success": False, | |
| "error": f"Unity instance '{unity_instance}' not found. Available: {[d.project for d in sessions.sessions.values()]}" | |
| }, status_code=404) | |
| if not session_id and not unity_instance: | |
| # Use first available session | |
| session_id = next(iter(sessions.sessions.keys())) |
🤖 Prompt for AI Agents
In @Server/src/main.py around lines 320 - 331, When a unity_instance is
specified but no matching session is found the code currently falls back to the
first session (session_id = next(...)), which can send commands to the wrong
instance; after the loop that sets session_id, if unity_instance is truthy and
session_id is still None, return a 404 error response (e.g., raise HTTPException
or abort with a clear message) instead of falling back—update the handler around
session_id/unity_instance/sessions.sessions to perform this check and
short-circuit with a 404 when the requested instance cannot be found.
1 participant
Hi team! The reason for this CLI is that the current MCP repo - even accelerated by some optimization - still needs sometime waiting and sometime can be undeterministic, with a lot of details of the calls and parameters hidden from the user's sight to save space. I am recently working on a game-based skill benchmark, and this skill caught my attention. To make the generation more automated, CLI seems to be an intuitive way and a new, faster approach to executing user commands.
Some advantages I can think of:
And some cons I can think of:
That would be all, looking forward to the team's feedback. Happy coding! @msanatan @dsarno
PS: Did a solid amount of AI coding with this one, then did throughful test myself (CLI tools are easy to test!)
Summary by Sourcery
Introduce a Click-based
unity-mcpCLI for interacting with the Unity MCP server over HTTP, including routes on the server to accept CLI commands and list Unity instances, along with configuration, output formatting, and tests.New Features:
unity-mcpcommand-line entry point with global options and multiple command groups for game objects, components, scenes, assets, scripts, editor control, prefabs, materials, lighting, audio, UI, animation, code, and raw commands.Enhancements:
Build:
unity-mcpconsole script inpyproject.tomland add Click as a runtime dependency.Documentation:
CLI_USAGE_GUIDE.md) describing installation, command structure, common pitfalls, and detailed command reference for AI assistants and developers.Tests:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.